fix: ESM / CJS interoperability issues with dependencies #34
Conversation
🦋 Changeset detectedLatest commit: 09b8b80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jorenbroekema
force-pushed
the
esm-cjs
branch
2 times, most recently
from
d5aa265 to
baa61cf
Compare
There was a problem hiding this comment.
Hey @jorenbroekema! I’m not 100% sure if we’ll accept this (it’s not code I’m familiar with so will leave that decision to someone else), but left some queries.
Just for transparency: AFAIK @astrojs/cli-kit is not really intended to be used outside of Astro, so it could well be that we decide not to accept changes if they don’t resolve issues we are ourselves facing. It might be worth checking out https://github.com/bombshell-dev/clack as an alternative tool.
| @@ -100,7 +102,6 @@ async function gradient( | |||
| stop() { | |||
| done = true; | |||
| stdin.removeListener("keypress", keypress); | |||
| clearInterval(interval); | |||
There was a problem hiding this comment.
Why was this removed? I’d expect it to be required still.
There was a problem hiding this comment.
The interval property was only initialized as let interval: NodeJS.Timeout; and then never used anywhere.
I'm assuming this was just a leftover of some old code that got removed.
TypeScript was complaining about it, I'm assuming the older version of TypeScript was slightly more lenient on it.
jorenbroekema
force-pushed
the
esm-cjs
branch
from
baa61cf to
09b8b80
Compare
Older versions of:
onetimerestore-cursorAre CJS-only. These are transitive dependencies caused by
log-update. Upgrading this one to latest, also ensures the latest versions of the above transitive dependencies are installed, and all of them publish ESM as well in these versions, so that's an easy fix.The other issue is
sisteransiwhich is CJS only and does not have an ESM publish, so we can just usecreateRequiretrick to import them in a manner that's compatible with NodeJS out of the box, not requiring build tools / bundlers to handle the interoperability by transforming the module from CJS -> ESM (which is error-prone)